Expose accessors for private/public key on elliptic curve keys #259

justsmth · 2023-10-24T14:32:30Z

A rebase of #234 onto the latest on main.

Description of changes:

Rebased changes from Expose accessors for private/public key on elliptic curve keys #234
Exposes a Ed25519Seed type.

mod signature {
    // Technically not exposed before (private type but visible via KeyPair impl on EcdsaKeyPair)
    pub struct EcdsaPublicKey { ... }
    pub struct EcdsaPrivateKey<'a> { ... }
    pub struct Ed25519Seed<'a> { ... }


    // These are buffer discriminants, not actual buffer types.
    pub struct EcPublicDer { ... }
    pub struct EcPrivateKeyBuffer { ... }
    pub struct EcPrivateKeyDer { ... }
    pub struct Ed25519SeedBuffer { ... }

    impl EcdsaKeyPair {
        fn generate(alg: &'static EcdsaSigningAlgorithm) -> Result<Self, Unspecified>;
        fn to_pkcs8v1(&self) -> Result<Document, Unspecified>;
        fn private_key(&self) -> EcdsaPrivateKey<'_>;
        fn from_private_key_der(alg: &'static EcdsaSigningAlgorithm, key: &[u8]) -> Result<Self, KeyRejected>;
    }
    impl EcdsaPrivateKey {
        fn to_buffer(&self) -> Result<Buffer<'static, EcPrivateKeyBuffer>, Unspecified>;
        fn to_der(&self) -> Result<Buffer<'static, EcPrivateKeyDer>, Unspecified>;
    }
    impl EcdsaPublicKey {
        /// Provides the public key as a DER-encoded SubjectPublicKeyInfo structure.
        fn as_der(&self) -> Buffer<'_, EcPublicDer>;
    }
    impl Ed25519Keypair {
        pub fn to_pkcs8v1(&self) -> Result<Document, Unspecified>;
        pub fn to_pkcs8v2(&self) -> Result<Document, Unspecified>;
        pub fn seed(&self) -> Result<Ed25519Seed, Unspecified>;
        impl Drop for Ed25519KeyPair { ... }
    }
    impl Ed25519Seed {
    	fn to_buffer(&self) -> Result<Buffer<'static, Ed25519SeedBuffer>, Unspecified>;
    }
}

mod buffer {
    pub struct Buffer<'a, T> { ... }
    impl<T> AsRef<[u8]> for Buffer<'_, T> { ... }
    impl<T> fmt::Debug for Buffer<'_, T> { ... }
}

Call-outs:

There’s inconsistency currently around whether to have a function names like to_pkcs8v1 versus to_pkcs8. Perhaps we need both?

(From #234)

We should discuss & agree to the general shape of the EcdsaKeyPair / EcdsaPrivateKey / EcdsaPublicKey types, as well as the new PrivateBuffer, to decide whether that shape makes sense. It's likely that replicating it for the other keys (e.g., RSA) will make sense. It's worth noting that the openssl crate takes a different approach with a general PKey type with functions to do a variety of operations -- that approach definitely leads to a simpler API, but it's less ring-like.

The current API surface is already a bit awkward -- for example, the only way to generate a key is via EcdsaKeyPair::generate_pkcs8 which gives you a serialized form rather than the Rust type. This PR adds a dedicated generate function, for latter use via to_pkcs8v1() if desired, but doesn't deprecate or remove the old API. We're also asking for a full signature algorithm rather than just the key type.

Testing:

(From #234)

A few unit tests are added. I'd like to agree on the general API shape before extending these further and it would help to gain some understanding of how much we want to test "trivial" wrappers around underlying AWS-LC functions for serialization/deserialization.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

Mark-Simulacrum

Just a couple tiny nits.

aws-lc-rs/src/ed25519.rs

codecov-commenter · 2023-10-24T17:31:28Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (95228a8) 95.42% compared to head (712f0f2) 95.47%.

Files	Patch %	Lines
aws-lc-rs/src/ec.rs	94.28%	4 Missing ⚠️
aws-lc-rs/src/ec/key_pair.rs	98.21%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #259      +/-   ##
==========================================
+ Coverage   95.42%   95.47%   +0.05%     
==========================================
  Files          51       52       +1     
  Lines        6733     6921     +188     
==========================================
+ Hits         6425     6608     +183     
- Misses        308      313       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

aws-lc-rs/src/buffer.rs

aws-lc-rs/src/ec.rs

skmcgrail · 2023-10-26T22:32:22Z

aws-lc-rs/src/ec.rs

-        PublicKey(pubkey_box)
+    /// Provides the public key as a DER-encoded `SubjectPublicKeyInfo` structure.
+    #[must_use]
+    pub fn as_der(&self) -> Buffer<'_, EcPublicKeyDer> {


I'm really not to fond of how we are using the names as_der and EcPublicKeyDer to mean "write the output as DER following the X.509 ASN.1 SubjectPublicKeyInfo type definition". DER is an ASN.1 encoding scheme, SubjectPublicKeyInfo is the structure to describe a public key that could be encoded in various formats, PEM, DER, BER, etc. I feel like we should be more explicit for own benefit in case we need to serialize to different structure types in the future.

Case in point, you could have a function that just writes the raw key bytes as a DER encode OCTET STRING. That's valid DER, but not a SubjectPublicKeyInfo.

Thoughts?

Would as_x509 and X509PublicKey any better?

As a reference, in the BC-FIPS Java world, the import of the der bytes (after base64 decoding if any) is something like:

new X509EncodedKeySpec(bytes)

skmcgrail · 2023-10-26T22:58:06Z

aws-lc-rs/src/ec/key_pair.rs

+    ///
+    /// # Errors
+    /// `error::Unspecified`  if serialization failed.
+    pub fn to_der(&self) -> Result<Buffer<'static, EcPrivateKeyDer>, Unspecified> {


Just wanted to highlight this as another example of ambiguity around meaning of to to_der. EcsdaPrivateKey encode to DER but follow PKCS#8 (v1 or v2) and is explicit with the functions. In this case though to_der is serializing only to the specific PrivateKey format, specifically from RFC 5915. This is the same structure that is wrapped by PKCS#8 structure! Now you could argue that KeyPair type makes sense to use PKCS#8 cause that supports both being present (V2). But it also supports serializing just the private key, so why does to_der here not use that? Basically I think the intent needs to be more clear for the serialization functions. I'm not saying we shouldn't support these types, but don't want to overload the term DER. ASN.1 is already confusing enough :)

I agree with your concerns. I'm still thinking about what changes need to be made regarding them.

Here are a few thoughts I have about what's important regarding the serialization/deserialization methods we provide for our structs:

Documentation should be unambiguous about the encoding(s) provided or required.

Method names should be clear about their purpose, but don't need to indicate details of the encoding.

Method names should be consistent/symmetric within a given struct. For example, the encoding serialized by a to_der function should be appropriate for a from_der to deserialize the same struct (if such a method exists).

Structs/Types (e.g., EcPrivateKeyDer) should be unambiguous and guide users toward proper usage of serialized data.

My latest commit adds/improves the documentation so that the encoding provided by serialization methods is more clear. I also moved the structs that differentiate buffer types to a signature::encoding module. We might also rename the (e.g.) to_der methods to more directly indicate the type of encoding provided, but I'm not yet sure that's needed.

This is a one-way-door API decision, so it should be carefully considered.

skmcgrail

Just some minor feedback and one question, otherwise this looks good.

aws-lc-rs/src/ed25519.rs

aws-lc-rs/src/ec/key_pair.rs

hansonchar

Instead of always marshaling a public key eagerly into both octet string and x509 formats, would it make more sense to have an API that allows either format to be lazily retrieved instead?

I can imagine cases where only one format is needed but not the other.

Also adds test coverage for EcdsaKeyPair::from_private_key_der and EcdsaPrivateKey::to_der (round trip test).

aws-lc-rs/src/buffer.rs

skmcgrail

Hopefully my final comment :)

aws-lc-rs/src/ec.rs

samuel40791765 · 2023-12-05T19:01:23Z

aws-lc-rs/src/ec.rs

-pub struct PublicKey(Box<[u8]>);
+pub struct PublicKey {
+    algorithm: &'static EcdsaSigningAlgorithm,
+    octets: Box<[u8]>,


Just a general question: We were doing this previously as well, so its not a blocker. We allocate the public key with a maximum length on the stack in marshal_public_key, does octets have to be a Box here?

samuel40791765 · 2023-12-05T19:16:18Z

aws-lc-rs/src/lib.rs

@@ -103,6 +103,7 @@ extern crate core;

 pub mod aead;
 pub mod agreement;
+mod buffer;


minor nit: Noticed this when checking if Buffer was a new public interface, should we move buffer down to list of other non-public modules below

I have another PR ready after this. I'll update the order there.

hansonchar · 2023-12-05T21:49:57Z

👏

justsmth requested a review from a team as a code owner October 24, 2023 14:32

Mark-Simulacrum reviewed Oct 24, 2023

View reviewed changes

aws-lc-rs/src/ed25519.rs Show resolved Hide resolved

aws-lc-rs/src/ed25519.rs Show resolved Hide resolved

justsmth force-pushed the ec-raw-api branch from e820e14 to e523567 Compare October 25, 2023 13:31

justsmth mentioned this pull request Oct 25, 2023

Expose accessors for private/public key on elliptic curve keys #234

Closed

justsmth force-pushed the ec-raw-api branch from e523567 to 2dc554d Compare October 25, 2023 13:40

skmcgrail reviewed Oct 26, 2023

View reviewed changes

justsmth force-pushed the ec-raw-api branch 3 times, most recently from 9ed166e to db45cd0 Compare October 27, 2023 16:04

skmcgrail requested review from skmcgrail and samuel40791765 October 31, 2023 17:21

skmcgrail reviewed Nov 6, 2023

View reviewed changes

aws-lc-rs/src/ed25519.rs Show resolved Hide resolved

aws-lc-rs/src/ec/key_pair.rs Outdated Show resolved Hide resolved

hansonchar reviewed Nov 7, 2023

View reviewed changes

This was referenced Nov 15, 2023

Add support for signature verification using public key in X509 DER encoding format. #274

Closed

Add support for signature verification using public key in X509 DER encoding format. #275

Closed

justsmth and others added 13 commits November 28, 2023 14:34

Support to_pkcs8

dd623a5

Access for EC keypair key bytes

8c710ea

Lifetime parameter for PrivateKey/Seed

247dca5

Expose private/public key ec types

84c3bf4

Expose PrivateKey::to_der

406b658

Add PublicKey::as_der

de85b90

Add EcdsaKeyPair::from_private_key_der

902ba3f

Fix version in comment

6edfd32

Refactor buffer to include per-type variants

d881f08

Also adds test coverage for EcdsaKeyPair::from_private_key_der and EcdsaPrivateKey::to_der (round trip test).

Clearer Ed25519KeyPair APIs

63b0031

Use d2i_PrivateKey instead of d2i_AutoPrivateKey

b70efd8

Add copyright for buffer.rs

3aea14a

API cleanup

1aa0684

justsmth added 6 commits November 28, 2023 14:34

Expose Ed25519Seed

78a2f40

Move Ed25519 private key into Box

0b72c89

Improve test coverage

0f3ead8

More increase test coverage

50edaf2

Update per PR comments

1c1cd8c

Buffer type aliases; Generate DER only when requested

d76698d

justsmth force-pushed the ec-raw-api branch from db45cd0 to d76698d Compare November 29, 2023 14:13

skmcgrail reviewed Nov 30, 2023

View reviewed changes

aws-lc-rs/src/buffer.rs Show resolved Hide resolved

Private buffer mod

f56c92b

justsmth force-pushed the ec-raw-api branch from c667372 to f56c92b Compare November 30, 2023 18:33

justsmth added 3 commits December 4, 2023 14:46

Traits: AsDer and AsBin

1b4c9d1

Cleanup; AsBigEndian

2bf0c10

Make clippy proud

16ca958

skmcgrail previously approved these changes Dec 4, 2023

View reviewed changes

aws-lc-rs/src/ec.rs Outdated Show resolved Hide resolved

skmcgrail self-requested a review December 4, 2023 23:43

Remove redundant copying

712f0f2

justsmth dismissed skmcgrail’s stale review via 712f0f2 December 5, 2023 16:49

skmcgrail approved these changes Dec 5, 2023

View reviewed changes

samuel40791765 approved these changes Dec 5, 2023

View reviewed changes

justsmth merged commit e6f69b1 into aws:main Dec 5, 2023
101 of 106 checks passed

justsmth deleted the ec-raw-api branch December 5, 2023 19:32

justsmth mentioned this pull request Jan 26, 2024

Update version to v1.6.0 #322

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose accessors for private/public key on elliptic curve keys #259

Expose accessors for private/public key on elliptic curve keys #259

justsmth commented Oct 24, 2023 •

edited

Loading

Mark-Simulacrum left a comment

codecov-commenter commented Oct 24, 2023 •

edited

Loading

skmcgrail Oct 26, 2023 •

edited

Loading

hansonchar Nov 6, 2023

skmcgrail Oct 26, 2023 •

edited

Loading

justsmth Oct 27, 2023

justsmth Oct 27, 2023

skmcgrail left a comment

hansonchar left a comment

skmcgrail left a comment

samuel40791765 Dec 5, 2023 •

edited

Loading

samuel40791765 Dec 5, 2023

justsmth Dec 5, 2023

hansonchar commented Dec 5, 2023

Expose accessors for private/public key on elliptic curve keys #259

Expose accessors for private/public key on elliptic curve keys #259

Conversation

justsmth commented Oct 24, 2023 • edited Loading

Description of changes:

Call-outs:

Testing:

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 24, 2023 • edited Loading

Codecov Report

skmcgrail Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

hansonchar Nov 6, 2023

Choose a reason for hiding this comment

skmcgrail Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

justsmth Oct 27, 2023

Choose a reason for hiding this comment

justsmth Oct 27, 2023

Choose a reason for hiding this comment

skmcgrail left a comment

Choose a reason for hiding this comment

hansonchar left a comment

Choose a reason for hiding this comment

skmcgrail left a comment

Choose a reason for hiding this comment

samuel40791765 Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

samuel40791765 Dec 5, 2023

Choose a reason for hiding this comment

justsmth Dec 5, 2023

Choose a reason for hiding this comment

hansonchar commented Dec 5, 2023

justsmth commented Oct 24, 2023 •

edited

Loading

codecov-commenter commented Oct 24, 2023 •

edited

Loading

skmcgrail Oct 26, 2023 •

edited

Loading

skmcgrail Oct 26, 2023 •

edited

Loading

samuel40791765 Dec 5, 2023 •

edited

Loading